Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: not prevent ipfs io requests #1230

Merged
merged 4 commits into from
Feb 9, 2022

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Feb 3, 2022

Needs:

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 3, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12c2925
Status: ✅  Deploy successful!
Preview URL: https://a31db549.nft-storage-1at.pages.dev

View logs

@vasco-santos vasco-santos changed the title Fix/not redirect nor prevent ipfs io requests fix: not redirect nor prevent ipfs io requests Feb 3, 2022
@vasco-santos vasco-santos changed the title fix: not redirect nor prevent ipfs io requests fix: not prevent ipfs io requests Feb 3, 2022
@vasco-santos vasco-santos force-pushed the fix/not-redirect-nor-prevent-ipfs-io-requests branch 2 times, most recently from 40791be to ebc6f59 Compare February 3, 2022 17:44
@@ -75,11 +75,6 @@ const MINUTE = 1000 * 60
*/
function getRateLimitingCharacteristics(gatewayUrl) {
switch (gatewayUrl) {
case 'https://ipfs.io':
return {
RATE_LIMIT_REQUESTS: 800,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just make this Infinity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I removed it entirely from calling this function to not have the delay of an async http request to the durable. Also, keeping infinity array size of timestamps might hit Limits of durable object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense...but...personally I would rather special case "Infinity" as a "limit" than a specific gateway. Maybe in the future we'll have other gateways that do not limit, if that happens we'll have to change code somewhere else than here and I'd rather just have a single place where we define the available gateways and their characteristics rather than scattered.

Unless the additional async request is problematic, I'd keep it there. It also makes the race a bit fairer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

Copy link
Contributor Author

@vasco-santos vasco-santos Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made 10 seconds the time to track, so that we do not have a really big array to track

@vasco-santos vasco-santos force-pushed the fix/not-redirect-nor-prevent-ipfs-io-requests branch 2 times, most recently from 5378c43 to ed55297 Compare February 8, 2022 16:17
@vasco-santos vasco-santos force-pushed the fix/not-redirect-nor-prevent-ipfs-io-requests branch from ed55297 to 31afa03 Compare February 8, 2022 17:10
@vasco-santos vasco-santos force-pushed the fix/not-redirect-nor-prevent-ipfs-io-requests branch from 31afa03 to f35e756 Compare February 8, 2022 17:13
@vasco-santos vasco-santos requested a review from alanshaw February 9, 2022 09:34
@vasco-santos vasco-santos marked this pull request as ready for review February 9, 2022 14:03
Copy link
Contributor

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@vasco-santos vasco-santos merged commit 769a465 into main Feb 9, 2022
@vasco-santos vasco-santos deleted the fix/not-redirect-nor-prevent-ipfs-io-requests branch February 9, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants